Skip to content

[api-extractor] Support deep link (e.g. for property type) - #1005

Closed
adventure-yunfei (adventure-yunfei) wants to merge 9 commits into
microsoft:masterfrom
adventure-yunfei:feature/AE7/deep-link
Closed

[api-extractor] Support deep link (e.g. for property type)#1005
adventure-yunfei (adventure-yunfei) wants to merge 9 commits into
microsoft:masterfrom
adventure-yunfei:feature/AE7/deep-link

Conversation

@adventure-yunfei

@adventure-yunfei adventure-yunfei (adventure-yunfei) commented Dec 22, 2018

Copy link
Copy Markdown
Contributor

(note: rush build and rush change is not executed yet.)

What does this PR do.

This PR is trying to support deep link for property type (class member, interface member, ...).

E.g. for following api.d.ts inside "demo" package:

export interface Bar {
}
export interface Foo {
    member: Bar;
}

api-extractor generates following part of api.json:

{
      "members": [
        {
          "kind": "Interface",
          "name": "Bar",
          "members": [],
          "excerptTokens": [
            {
              "kind": "Content",
              "text": "export interface "
            },
            {
              "kind": "Reference",
              "text": "Bar",
              "reference": "demo#Bar"
            },
            {
              "kind": "Content",
              "text": " "
            }
          ],
        },
        {
          "kind": "Interface",
          "name": "Foo",
          "members": [
            {
              "kind": "PropertySignature",
              "name": "member",
              "excerptTokens": [
                {
                  "kind": "Reference",
                  "text": "member",
                  "reference": "demo#Foo.member"
                },
                {
                  "kind": "Content",
                  "text": ": "
                },
                {
                  "kind": "Reference",
                  "text": "Bar",
                  "reference": "demo#Bar"
                },
                {
                  "kind": "Content",
                  "text": ";"
                }
              ],
            }
          ],
          "excerptTokens": [
            {
              "kind": "Content",
              "text": "export interface "
            },
            {
              "kind": "Reference",
              "text": "Foo",
              "reference": "demo#Foo"
            },
            {
              "kind": "Content",
              "text": " "
            }
          ],
        }
      ]
}

And the api-documenter generatos the demo.foo.md as following:

......
## Properties

|  <p>Property</p> | <p>Type</p> | <p>Description</p> |
|  --- | --- | --- |
|  <p>[member](./demo.foo.member.md)</p> | <p>[Bar](./demo.bar.md)</p> |  |
......

The type of property member of interface Foo can now be linked to interface Bar page.

How it is done.

  1. When generating api items in ApiModelGenerator.ts, record them for each ts.Symbol
  2. When generating excerpt token for an api item, if an identifier is found, resolve its referring ts.Symbol and record.
  3. After processing, with previous recorded info, update the excerpt token reference to the actual api item path by tsdoc DocDeclarationReference node format.

Pending implementation

  • handle the case that a symbol can have multiple declarations (DocMemberReference.selector field)
  • test with different cases
  • Support reference for un-exported local items and external package items.

@octogonz

Pete Gonzalez (octogonz) commented Dec 28, 2018

Copy link
Copy Markdown
Collaborator

adventure-yunfei (@adventure-yunfei) thanks for contributing this PR! I apologize that no one has looked at it yet. We are out of office for holidays this week and next week. This PR review is somewhat involved, since it is a planned feature, but the design is not fully worked out yet. I may not be able to give in depth feedback until I'm back in the office.

Briefly, the intended design was to rely on the declaration reference concept from TSDoc. This notation handles multiple declarations such as overloaded functions and merged declarations. (There are a bunch of notation examples here.) API Extractor's declaration references are generated using ApiItem.canonicalReference, and parsed using ApiModel.resolveDeclarationReference(). As you mentioned, when dealing with symbols exported from an external package, there is a problem of determining the appropriate entry point. The tentative plan was to solve this using the tsdoc-metadata.json data file which was proposed in TSDoc RFC #7. It's a little complicated, so we were going to tackle this after version 7 is released.

If you want to help push this feature along more quickly, that's much appreciated!

@adventure-yunfei

Copy link
Copy Markdown
Contributor Author

Pete Gonzalez (OLD ALIAS) (@pgonzal) thanks for explanation. I've noticed that tsdoc link is handled in api-extractor, but only for some cases now. I'll dig into your tsdoc info first.

@octogonz

Copy link
Copy Markdown
Collaborator

(I wasn't sure about the status of this PR, so I've tagged it as "PR: Waiting for author." If you're ready for me to look at it again, please remove that tag and add "PR: Ready for review")

@octogonz Pete Gonzalez (octogonz) changed the title [api-extractor7] support deep link (e.g. for property type) [api-extractor] Support deep link (e.g. for property type) Jan 9, 2019
@adventure-yunfei

Copy link
Copy Markdown
Contributor Author

I'll close this PR as there should be a better way.

Luckily I can use this code inside my own branch. I've found a way to support the local file module re-export. With this change the api-documenter generates a perfect api site for our own projects right now~

@adventure-yunfei

Copy link
Copy Markdown
Contributor Author

Pete Gonzalez (@octogonz) I've created a new version from latest mater and force-pushed the branch. I'm willing to reopen this PR.
The main change is to use tsdoc DocDeclarationReference format instead of getScopedNameWithinPackage() to represent the api item reference.

@octogonz

Copy link
Copy Markdown
Collaborator

Thanks! I finally got PR #1077 completed and merged (which was a somewhat big undertaking), so this week I'm going to try to clear through the backlog of other PRs.

…/deep-link

# Conflicts:
#	apps/api-extractor/src/generators/ApiModelGenerator.ts
@octogonz

Copy link
Copy Markdown
Collaborator

After building your branch, I noticed a problem with the .api.json for this declaration:

build-tests/api-extractor-scenarios/src/circularImport2/IFile.ts

import { IFolder } from "./IFolder";

/** @public */
export class IFile {
  containingFolder: IFolder;
}

The corresponding .api.json looks like this:

{
  "kind": "Class",
  "canonicalReference": "(IFile:class)",
  "docComment": "/**\n * @public\n */\n",
  "excerptTokens": [
    {
      "kind": "Content",
      "text": "export declare class "
    },
    {
      "kind": "Reference",
      "text": "IFile",
      "reference": "api-extractor-scenarios#IFile"
    },
    {
      "kind": "Content",
      "text": " "
    }
  ],
  "releaseTag": "Public",
  "name": "IFile",
  "members": [
    {
      "kind": "Property",
      "canonicalReference": "(containingFolder:instance)",
      "docComment": "",
      "excerptTokens": [
        {
          "kind": "Reference",
          "text": "containingFolder",
          "reference": "api-extractor-scenarios#IFile.containingFolder"  // <--- seems incorrect
        },
        {
          "kind": "Content",
          "text": ": "
        },
        {
          "kind": "Reference",
          "text": "IFolder"  // <--- no "reference" field
        },
        {
          "kind": "Content",
          "text": ";"
        }
      ],
      "releaseTag": "Public",
      "name": "containingFolder",
      "propertyTypeTokenRange": {
        "startIndex": 2,
        "endIndex": 3
      },
      "isStatic": false
    }
  ],
  "implementsTokenRanges": []
},

If I get some time I'll see if I can debug it. Sorry it's taken me a while to finally look at this PR -- I've been very focused on the AE7 release.

@adventure-yunfei

adventure-yunfei (adventure-yunfei) commented Mar 10, 2019

Copy link
Copy Markdown
Contributor Author

"reference": "api-extractor-scenarios#IFile.containingFolder" // <--- seems incorrect

The path is the api item reference path, it's referring to the IFile.containingFolder item inside api-extractor-scenarios package (the exported api.json shows this hierachy). I would think it's correct.

"text": "IFolder" // <--- no "reference" field

This is indeed a mistake, IFolder is exported but not found for the reference token. Since the link between reference token and api item is the same ts symbol, I guess there may be an alias making them pointing to different symbol. I'll try to make time to check that (but currently I'm busy with my work, it may take some time.)

@adventure-yunfei

Copy link
Copy Markdown
Contributor Author

Pete Gonzalez (@octogonz) Sorry that I didn't have time to check this util now.
Confirmed that the second case "text": "IFolder" // <--- no "reference" field is caused by aliased symbol, and fixed in 8d9382f .

@adventure-yunfei

adventure-yunfei (adventure-yunfei) commented May 20, 2019

Copy link
Copy Markdown
Contributor Author

updated with some code optimization and "rush build" results.
(I've uncommented this code ccfbb5c to remove declaration link text, as it's not displayed well.)

And I've tested api-documenter-test and api-extractor-scenarios. All reference links are correctly displayed (except for un-exported ones, which is not handled in the MR).

  • Demonstrate links in declaration text (temporarily enabled for test):
    image

  • Demonstrate links in type:
    image

@octogonz

Copy link
Copy Markdown
Collaborator

Cool, thanks for making these updates! I will take a look as soon as I can. (Lately we're having trouble keeping up with all the activity in this repo! 😁)

@octogonz

Copy link
Copy Markdown
Collaborator

The latest work on this feature is in PR #1337. That PR is focused on the DocFX/Yaml target. It does not yet address the Markdown target, but the approach will be the same.

We will probably close this PR, but I'd like to study it first to make sure the work here isn't useful.

@octogonz

Copy link
Copy Markdown
Collaborator

adventure-yunfei (@adventure-yunfei) thanks for contributing this PR! As discussed we've decided to close it in favor of Ron Buckton (@rbuckton)'s PR #1337, which expands on this idea with a more comprehensive solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants